-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[RyuJIT Wasm] Add Object Writer Imports Section + Constant Exprs to Encode R2R Payload Placement #123739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[RyuJIT Wasm] Add Object Writer Imports Section + Constant Exprs to Encode R2R Payload Placement #123739
Conversation
…k pointer, and r2r start address by default
Lay out R2R data segments relative to imported __r2r_start global
|
I will try and update the test harness to be compatible with this |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
|
@kg I'd love a review on this when you have a chance! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds initial support for emitting a Wasm import section from WasmObjectWriter, and switches R2R data segment placement to use constant expressions based on an imported __r2r_start symbol.
Changes:
- Introduces a new
wasm.importobject node section and emits it as Wasm section typeImport. - Reworks combined data segment placement to use an instruction expression (
global.get __r2r_start+i32.const offset+i32.add) instead of a fixedDataStartOffset. - Adds a small Wasm encoding model (imports, globals, memory type, expression/instruction encoding) and a helper to write UTF-8 strings with a length prefix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs | Adds import section emission and uses __r2r_start + offset constant expressions for data segment placement. |
| src/coreclr/tools/Common/Compiler/ObjectWriter/WasmNative.cs | Adds encodable abstractions/types for Wasm expressions and import encodings (memory/global types, etc.). |
| src/coreclr/tools/Common/Compiler/ObjectWriter/SectionWriter.cs | Adds WriteUtf8WithLength helper for Wasm-style length-prefixed strings. |
| class WasmInstructionGroup : IWasmEncodable | ||
| { | ||
| readonly WasmExpr[] WasmExprs; | ||
| public WasmInstructionGroup(WasmExpr[] wasmExprs) | ||
| { | ||
| WasmExprs = wasmExprs; | ||
| } |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WasmInstructionGroup uses a field named WasmExprs without the underscore prefix used elsewhere in this file (e.g., _types, _params, _returns). Rename to _wasmExprs (and update usages) to match the established local naming convention and improve readability.
| public WasmMemoryType(WasmLimitType limitType, uint min, uint? max = null) | ||
| { | ||
| if (LimitType == WasmLimitType.HasMinAndMax && !Max.HasValue) | ||
| { | ||
| throw new ArgumentException("Max must be provided when LimitType is HasMinAndMax"); | ||
| } | ||
|
|
||
| LimitType = limitType; | ||
| Min = min; | ||
| Max = max; | ||
| } | ||
|
|
||
| public override int Encode(Span<byte> buffer) | ||
| { | ||
| int pos = 0; | ||
| buffer[pos++] = (byte)LimitType; | ||
| pos += DwarfHelper.WriteULEB128(buffer.Slice(pos), Min); | ||
| if (LimitType == WasmLimitType.HasMinAndMax) | ||
| { | ||
| pos += DwarfHelper.WriteULEB128(buffer.Slice(pos), Max!.Value); | ||
| } | ||
| return pos; | ||
| } | ||
|
|
||
| public override int EncodeSize() => 2; | ||
| } |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WasmMemoryType has two issues:
- The constructor validates
LimitType/Maxfields before they’re assigned, so the check never does what it intends. It should validate thelimitType/maxparameters. EncodeSize()returns a constant2, butEncode(...)writes a variable-length payload (1 byte + ULEB128(min) [+ ULEB128(max)]). This will cause callers usingEncodeSize()(e.g., import encoding) to allocate the wrong buffer size and can lead to truncated writes/corruption. Compute the size usingDwarfHelper.SizeOfULEB128formin/maxand include the leading limit-type byte.
| int size = import.EncodeSize(); | ||
| import.Encode(writer.Buffer.GetSpan(size)); | ||
| writer.Buffer.Advance((int)size); |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WriteImport advances the section buffer by EncodeSize() rather than by the actual byte count returned from Encode(...). If EncodeSize() and Encode() ever diverge (or a future implementation writes fewer bytes), this will either leave garbage bytes in the section or corrupt subsequent writes. Capture the return value from Encode(...), assert it matches the expected size, and advance by the encoded byte count.
| int size = import.EncodeSize(); | |
| import.Encode(writer.Buffer.GetSpan(size)); | |
| writer.Buffer.Advance((int)size); | |
| int expectedSize = import.EncodeSize(); | |
| Span<byte> buffer = writer.Buffer.GetSpan(expectedSize); | |
| int encodedSize = import.Encode(buffer); | |
| Debug.Assert(encodedSize == expectedSize, $"Encoded import size mismatch in {nameof(WriteImport)}."); | |
| writer.Buffer.Advance(encodedSize); |
| WasmInstructionGroup GetR2StartOffset(int offset) | ||
| { | ||
| return new WasmInstructionGroup([ | ||
| Global.Get(1), // __r2r_start | ||
| I32.Const(offset), | ||
| I32.Add, | ||
| ]); |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetR2StartOffset hard-codes Global.Get(1) for __r2r_start. This couples data segment placement to the current import/global ordering and will silently break if another imported/defined global is added earlier. Consider tracking the global index for __r2r_start when writing imports (e.g., via constants/enums or by computing the index from the import list) and use that value here.
| ulong numPages = (contentSize + (1<<16) - 1) >> 16; | ||
|
|
||
| _defaultImports[0] = new WasmImport("env", "memory", WasmExternalKind.Memory, | ||
| new WasmMemoryType(0x00, (uint)numPages)); // memory limits: flags (0 = only minimum) |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new WasmMemoryType(0x00, (uint)numPages) passes an integer literal where WasmMemoryType expects a WasmLimitType, which will not compile. Use WasmLimitType.HasMin (or an explicit cast) for the first argument.
| new WasmMemoryType(0x00, (uint)numPages)); // memory limits: flags (0 = only minimum) | |
| new WasmMemoryType(WasmLimitType.HasMin, (uint)numPages)); // memory limits: flags (HasMin = only minimum) |
| // Calculate the required memory size based on the combined data section size | ||
| ulong contentSize = (ulong)SectionByName(WasmObjectNodeSection.CombinedDataSection.Name).ContentSize; | ||
| ulong numPages = (contentSize + (1<<16) - 1) >> 16; | ||
|
|
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numPages is computed only from the combined data section’s encoded size. This can (a) produce a 0-page minimum when there are no data segments, and (b) ignores any non-zero base placement such as the previous DataStartOffset (and now potentially __r2r_start). Since the import’s minimum is the module’s only enforcement on the host-provided memory size, consider clamping to at least 1 page and/or adding a fixed base reservation (stack / __r2r_start placement) so the requested minimum can’t be smaller than the runtime’s expected address range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
| } | ||
|
|
||
| // Simple DSL wrapper for creating Wasm expressions | ||
| static class Global |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put the DSL in a namespace of its own
| private WasmDataSection CreateCombinedDataSection(int dataStartOffset) | ||
| private WasmDataSection CreateCombinedDataSection() | ||
| { | ||
| WasmInstructionGroup GetR2StartOffset(int offset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be GetR2RStartOffset
|
This looks fairly good, all of Copilot's feedback seemed accurate though so we need to address it. |
This PR adds support for generating an import section to the WasmObjectWriter. We currently import:
__stack_pointer__r2r_startVarious data segments are then placed into
__r2r_start + <segment_offset>. We use the constant expressions extension for these address calculations.